Fix CFI initialization crash on x86.

Bug: 34752378
Test: bionic tests
Change-Id: If8e33f76a1a2d83356d818fed506ea624f579860

10 files changed, 251 insertions, 50 deletions

diff --git a/libdl/libdl_cfi.cpp b/libdl/libdl_cfi.cpp
index 362b093fa..8458564a8 100644
--- a/libdl/libdl_cfi.cpp
+++ b/libdl/libdl_cfi.cpp
@@ -29,10 +29,12 @@ static struct {
   char padding[PAGE_SIZE - sizeof(v)];
 } shadow_base_storage alignas(PAGE_SIZE);
 
+// __cfi_init is called by the loader as soon as the shadow is mapped. This may happen very early
+// during startup, before libdl.so global constructors, and, on i386, even before __libc_sysinfo is
+// initialized. This function should not do any system calls.
 extern "C" uintptr_t* __cfi_init(uintptr_t shadow_base) {
   shadow_base_storage.v = shadow_base;
   static_assert(sizeof(shadow_base_storage) == PAGE_SIZE, "");
-  mprotect(&shadow_base_storage, PAGE_SIZE, PROT_READ);
   return &shadow_base_storage.v;
 }
 
diff --git a/linker/linker_cfi.cpp b/linker/linker_cfi.cpp
index e9cdab64a..28d2eaf02 100644
--- a/linker/linker_cfi.cpp
+++ b/linker/linker_cfi.cpp
@@ -193,6 +193,7 @@ bool CFIShadowWriter::NotifyLibDl(soinfo* solist, uintptr_t p) {
   shadow_start = reinterpret_cast<uintptr_t* (*)(uintptr_t)>(cfi_init)(p);
   CHECK(shadow_start != nullptr);
   CHECK(*shadow_start == p);
+  mprotect(shadow_start, PAGE_SIZE, PROT_READ);
   return true;
 }
 
diff --git a/tests/cfi_test.cpp b/tests/cfi_test.cpp
index 0f93edb14..a73fd34e1 100644
--- a/tests/cfi_test.cpp
+++ b/tests/cfi_test.cpp
@@ -1,7 +1,25 @@
-#include <gtest/gtest.h>
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 #include <dlfcn.h>
+#include <gtest/gtest.h>
 
 #include "BionicDeathTest.h"
+#include "gtest_globals.h"
+#include "utils.h"
 
 // Private libdl interface.
 extern "C" {
@@ -92,3 +110,24 @@ TEST(cfi_test, invalid) {
   handle = dlopen("libcfi-test-bad.so", RTLD_NOW | RTLD_LOCAL);
   ASSERT_FALSE(handle != nullptr) << dlerror();
 }
+
+// cfi_test_helper exports __cfi_check, which triggers CFI initialization at startup.
+TEST(cfi_test, early_init) {
+#if defined(__BIONIC__)
+  std::string helper = get_testlib_root() + "/cfi_test_helper/cfi_test_helper";
+  ExecTestHelper eth;
+  eth.SetArgs({ helper.c_str(), nullptr });
+  eth.Run([&]() { execve(helper.c_str(), eth.GetArgs(), eth.GetEnv()); }, 0, nullptr);
+#endif
+}
+
+// cfi_test_helper2 depends on a library that exports __cfi_check, which triggers CFI initialization
+// at startup.
+TEST(cfi_test, early_init2) {
+#if defined(__BIONIC__)
+  std::string helper = get_testlib_root() + "/cfi_test_helper2/cfi_test_helper2";
+  ExecTestHelper eth;
+  eth.SetArgs({ helper.c_str(), nullptr });
+  eth.Run([&]() { execve(helper.c_str(), eth.GetArgs(), eth.GetEnv()); }, 0, nullptr);
+#endif
+}
diff --git a/tests/libs/Android.bp b/tests/libs/Android.bp
index b5855af62..9f9c40724 100644
--- a/tests/libs/Android.bp
+++ b/tests/libs/Android.bp
@@ -494,3 +494,20 @@ cc_test_library {
         cfi: false,
     },
 }
+
+cc_test {
+    name: "cfi_test_helper",
+    host_supported: false,
+    defaults: ["bionic_testlib_defaults"],
+    srcs: ["cfi_test_helper.cpp"],
+    ldflags: [" -Wl,-export-dynamic-symbol=__cfi_check"],
+}
+
+cc_test {
+    name: "cfi_test_helper2",
+    host_supported: false,
+    defaults: ["bionic_testlib_defaults"],
+    srcs: ["cfi_test_helper2.cpp"],
+    shared_libs: ["libcfi-test"],
+    ldflags: ["-Wl,--rpath,${ORIGIN}/.."],
+}
diff --git a/tests/libs/cfi_test_helper.cpp b/tests/libs/cfi_test_helper.cpp
new file mode 100644
index 000000000..ff313a2d8
--- /dev/null
+++ b/tests/libs/cfi_test_helper.cpp
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <assert.h>
+#include <stdint.h>
+#include <stdlib.h>
+
+#include "libs_utils.h"
+
+// This library is built for all targets, including host tests, so __cfi_slowpath may not be
+// present. But it is only used in the bionic loader tests.
+extern "C" __attribute__((weak)) void __cfi_slowpath(uint64_t, void*);
+
+static int g_count;
+
+// Mock a CFI-enabled library without relying on the compiler.
+extern "C" __attribute__((aligned(4096))) void __cfi_check(uint64_t /*CallSiteTypeId*/,
+                                                           void* /*TargetAddr*/, void* /*Diag*/) {
+  ++g_count;
+}
+
+void preinit_ctor() {
+  CHECK(g_count == 0);
+  __cfi_slowpath(42, reinterpret_cast<void*>(&preinit_ctor));
+  CHECK(g_count == 1);
+}
+
+__attribute__((section(".preinit_array"), used)) void (*preinit_ctor_p)(void) = preinit_ctor;
+
+__attribute__((constructor, used)) void ctor() {
+  CHECK(g_count == 1);
+  __cfi_slowpath(42, reinterpret_cast<void*>(&ctor));
+  CHECK(g_count == 2);
+}
+
+int main(void) {
+  CHECK(g_count == 2);
+  __cfi_slowpath(42, reinterpret_cast<void*>(&main));
+  CHECK(g_count == 3);
+  return 0;
+}
diff --git a/tests/libs/cfi_test_helper2.cpp b/tests/libs/cfi_test_helper2.cpp
new file mode 100644
index 000000000..11a6036c0
--- /dev/null
+++ b/tests/libs/cfi_test_helper2.cpp
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <assert.h>
+#include <dlfcn.h>
+
+#include "libs_utils.h"
+
+int main(void) {
+  void* handle;
+  // libcfi-test.so does some basic testing in a global constructor. Check that it is linked.
+  handle = dlopen("libcfi-test.so", RTLD_NOW | RTLD_NOLOAD);
+  CHECK(handle != nullptr);
+  dlclose(handle);
+  return 0;
+}
diff --git a/tests/libs/cfi_test_lib.cpp b/tests/libs/cfi_test_lib.cpp
index b0e2f426d..959b1020f 100644
--- a/tests/libs/cfi_test_lib.cpp
+++ b/tests/libs/cfi_test_lib.cpp
@@ -1,3 +1,19 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
 #include <assert.h>
 #include <stdint.h>
 #include <stdlib.h>
diff --git a/tests/libs/libs_utils.h b/tests/libs/libs_utils.h
new file mode 100644
index 000000000..f11cbe782
--- /dev/null
+++ b/tests/libs/libs_utils.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef LIBS_UTILS_H
+#define LIBS_UTILS_H
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#define CHECK(x)                                                            \
+  do {                                                                      \
+    fprintf(stderr, "CHECK(" #x ") failed at %s:%d\n", __FILE__, __LINE__); \
+    if (!(x)) abort();                                                      \
+  } while (0)
+
+#endif  // LIBS_UTILS_H
diff --git a/tests/unistd_test.cpp b/tests/unistd_test.cpp
index 9d809f065..ddf0df5bf 100644
--- a/tests/unistd_test.cpp
+++ b/tests/unistd_test.cpp
@@ -1203,52 +1203,6 @@ TEST(UNISTD_TEST, setdomainname) {
   }
 }
 
-class ExecTestHelper {
- public:
-  char** GetArgs() { return const_cast<char**>(args_.data()); }
-  char** GetEnv() { return const_cast<char**>(env_.data()); }
-
-  void SetArgs(const std::vector<const char*> args) { args_ = args; }
-  void SetEnv(const std::vector<const char*> env) { env_ = env; }
-
-  void Run(const std::function<void ()>& child_fn,
-           int expected_exit_status,
-           const char* expected_output) {
-      int fds[2];
-      ASSERT_NE(pipe2(fds, 0), -1);
-
-      pid_t pid = fork();
-      ASSERT_NE(pid, -1);
-
-      if (pid == 0) {
-          // Child.
-          close(fds[0]);
-          dup2(fds[1], STDOUT_FILENO);
-          dup2(fds[1], STDERR_FILENO);
-          if (fds[1] != STDOUT_FILENO && fds[1] != STDERR_FILENO) close(fds[1]);
-          child_fn();
-          FAIL();
-      }
-
-      // Parent.
-      close(fds[1]);
-      std::string output;
-      char buf[BUFSIZ];
-      ssize_t bytes_read;
-      while ((bytes_read = TEMP_FAILURE_RETRY(read(fds[0], buf, sizeof(buf)))) > 0) {
-          output.append(buf, bytes_read);
-      }
-      close(fds[0]);
-
-      AssertChildExited(pid, expected_exit_status);
-      ASSERT_EQ(expected_output, output);
-  }
-
- private:
-  std::vector<const char*> args_;
-  std::vector<const char*> env_;
-};
-
 #if defined(__GLIBC__)
 #define BIN_DIR "/bin/"
 #else
diff --git a/tests/utils.h b/tests/utils.h
index 79eed10b9..0b9b886a1 100644
--- a/tests/utils.h
+++ b/tests/utils.h
@@ -125,8 +125,13 @@ static inline void WaitUntilThreadSleep(std::atomic<pid_t>& tid) {
 static inline void AssertChildExited(int pid, int expected_exit_status) {
   int status;
   ASSERT_EQ(pid, waitpid(pid, &status, 0));
-  ASSERT_TRUE(WIFEXITED(status));
-  ASSERT_EQ(expected_exit_status, WEXITSTATUS(status));
+  if (expected_exit_status >= 0) {
+    ASSERT_TRUE(WIFEXITED(status));
+    ASSERT_EQ(expected_exit_status, WEXITSTATUS(status));
+  } else {
+    ASSERT_TRUE(WIFSIGNALED(status));
+    ASSERT_EQ(-expected_exit_status, WTERMSIG(status));
+  }
 }
 
 // The absolute path to the executable
@@ -142,4 +147,59 @@ int get_argc();
 char** get_argv();
 char** get_envp();
 
+class ExecTestHelper {
+ public:
+  char** GetArgs() {
+    return const_cast<char**>(args_.data());
+  }
+  char** GetEnv() {
+    return const_cast<char**>(env_.data());
+  }
+
+  void SetArgs(const std::vector<const char*> args) {
+    args_ = args;
+  }
+  void SetEnv(const std::vector<const char*> env) {
+    env_ = env;
+  }
+
+  void Run(const std::function<void()>& child_fn, int expected_exit_status,
+           const char* expected_output) {
+    int fds[2];
+    ASSERT_NE(pipe2(fds, 0), -1);
+
+    pid_t pid = fork();
+    ASSERT_NE(pid, -1);
+
+    if (pid == 0) {
+      // Child.
+      close(fds[0]);
+      dup2(fds[1], STDOUT_FILENO);
+      dup2(fds[1], STDERR_FILENO);
+      if (fds[1] != STDOUT_FILENO && fds[1] != STDERR_FILENO) close(fds[1]);
+      child_fn();
+      FAIL();
+    }
+
+    // Parent.
+    close(fds[1]);
+    std::string output;
+    char buf[BUFSIZ];
+    ssize_t bytes_read;
+    while ((bytes_read = TEMP_FAILURE_RETRY(read(fds[0], buf, sizeof(buf)))) > 0) {
+      output.append(buf, bytes_read);
+    }
+    close(fds[0]);
+
+    AssertChildExited(pid, expected_exit_status);
+    if (expected_output != nullptr) {
+      ASSERT_EQ(expected_output, output);
+    }
+  }
+
+ private:
+  std::vector<const char*> args_;
+  std::vector<const char*> env_;
+};
+
 #endif