From 26ec9679ff01fb155ae21015f31cc95bed24f670 Mon Sep 17 00:00:00 2001 From: "Torne (Richard Coles)" Date: Wed, 30 Apr 2014 15:48:40 +0100 Subject: [PATCH] Handle empty relro segment or incorrectly sized file. If the file has no relro segment, the generated relro file will have length 0, which caused mmap to fail. If the relro file has nonzero size, but is too short (e.g. because it's for the wrong version of the library), the linker would segfault while comparing the data. Fix both these issues: don't try to map a zero length file, and don't try to compare data that would be beyond the end of the file. Improve test to explicitly generate two versions of the library: one with -z relro, and one with -z norelro, so we can test both cases; also explicitly test the case where the relro file has length 0. Bug: 14299541 Change-Id: Id8b95585edda90e8bb5de452a35b70ed2d224934 --- linker/linker_phdr.cpp | 16 +++++- tests/Android.mk | 17 +++++- tests/dlext_test.cpp | 123 +++++++++++++++++++++++++++-------------- 3 files changed, 109 insertions(+), 47 deletions(-) diff --git a/linker/linker_phdr.cpp b/linker/linker_phdr.cpp index cfeab967b..12e977940 100644 --- a/linker/linker_phdr.cpp +++ b/linker/linker_phdr.cpp @@ -591,9 +591,12 @@ int phdr_table_map_gnu_relro(const ElfW(Phdr)* phdr_table, size_t phdr_count, El return -1; } off_t file_size = file_stat.st_size; - void* temp_mapping = mmap(NULL, file_size, PROT_READ, MAP_PRIVATE, fd, 0); - if (temp_mapping == MAP_FAILED) { - return -1; + void* temp_mapping = NULL; + if (file_size > 0) { + temp_mapping = mmap(NULL, file_size, PROT_READ, MAP_PRIVATE, fd, 0); + if (temp_mapping == MAP_FAILED) { + return -1; + } } size_t file_offset = 0; @@ -614,6 +617,13 @@ int phdr_table_map_gnu_relro(const ElfW(Phdr)* phdr_table, size_t phdr_count, El size_t match_offset = 0; size_t size = seg_page_end - seg_page_start; + if (file_size - file_offset < size) { + // File is too short to compare to this segment. The contents are likely + // different as well (it's probably for a different library version) so + // just don't bother checking. + break; + } + while (match_offset < size) { // Skip over dissimilar pages. while (match_offset < size && diff --git a/tests/Android.mk b/tests/Android.mk index 0014af616..c6fa54e4a 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -190,17 +190,32 @@ include $(LOCAL_PATH)/Android.build.mk endif # ----------------------------------------------------------------------------- -# Library used by dlext tests. +# Library used by dlext tests - with/without GNU RELRO program header # ----------------------------------------------------------------------------- libdlext_test_src_files := \ dlext_test_library.cpp \ +libdlext_test_ldflags := \ + -Wl,-z,relro \ + module := libdlext_test module_tag := optional build_type := target build_target := SHARED_LIBRARY include $(LOCAL_PATH)/Android.build.mk +libdlext_test_norelro_src_files := \ + dlext_test_library.cpp \ + +libdlext_test_norelro_ldflags := \ + -Wl,-z,norelro \ + +module := libdlext_test_norelro +module_tag := optional +build_type := target +build_target := SHARED_LIBRARY +include $(LOCAL_PATH)/Android.build.mk + # ----------------------------------------------------------------------------- # Tests for the device using bionic's .so. Run with: # adb shell /data/nativetest/bionic-unit-tests/bionic-unit-tests diff --git a/tests/dlext_test.cpp b/tests/dlext_test.cpp index 872cc5b36..4b2a5e23c 100644 --- a/tests/dlext_test.cpp +++ b/tests/dlext_test.cpp @@ -39,6 +39,7 @@ typedef int (*fn)(void); #define LIBNAME "libdlext_test.so" +#define LIBNAME_NORELRO "libdlext_test_norelro.so" #define LIBSIZE 1024*1024 // how much address space to reserve for it @@ -144,52 +145,88 @@ TEST_F(DlExtTest, ReservedHintTooSmall) { EXPECT_EQ(4, f()); } -TEST_F(DlExtTest, RelroShareChildWrites) { - void* start = mmap(NULL, LIBSIZE, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, - -1, 0); - ASSERT_TRUE(start != MAP_FAILED); - android_dlextinfo extinfo; - extinfo.reserved_addr = start; - extinfo.reserved_size = LIBSIZE; +class DlExtRelroSharingTest : public DlExtTest { +protected: + virtual void SetUp() { + DlExtTest::SetUp(); + void* start = mmap(NULL, LIBSIZE, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, + -1, 0); + ASSERT_TRUE(start != MAP_FAILED); + extinfo_.flags = ANDROID_DLEXT_RESERVED_ADDRESS; + extinfo_.reserved_addr = start; + extinfo_.reserved_size = LIBSIZE; + extinfo_.relro_fd = -1; - int relro_fd; - char relro_file[PATH_MAX]; - const char* android_data = getenv("ANDROID_DATA"); - ASSERT_TRUE(android_data != NULL); - snprintf(relro_file, sizeof(relro_file), "%s/local/tmp/libdlext_test.relro", android_data); - relro_fd = open(relro_file, O_CREAT | O_RDWR | O_TRUNC, 0644); - extinfo.flags = ANDROID_DLEXT_RESERVED_ADDRESS | ANDROID_DLEXT_WRITE_RELRO; - ASSERT_NOERROR(relro_fd); - extinfo.relro_fd = relro_fd; - - pid_t pid = fork(); - if (pid == 0) { - // child process - void* handle = android_dlopen_ext(LIBNAME, RTLD_NOW, &extinfo); - if (handle == NULL) { - fprintf(stderr, "in child: %s\n", dlerror()); - exit(1); - } - exit(0); + const char* android_data = getenv("ANDROID_DATA"); + ASSERT_TRUE(android_data != NULL); + snprintf(relro_file_, sizeof(relro_file_), "%s/local/tmp/libdlext_test.relro", android_data); } - // continuing in parent - ASSERT_NOERROR(close(relro_fd)); - ASSERT_NOERROR(pid); - int status; - ASSERT_EQ(pid, waitpid(pid, &status, 0)); - ASSERT_TRUE(WIFEXITED(status)); - ASSERT_EQ(0, WEXITSTATUS(status)); + virtual void TearDown() { + DlExtTest::TearDown(); + if (extinfo_.relro_fd != -1) { + ASSERT_NOERROR(close(extinfo_.relro_fd)); + } + } - relro_fd = open(relro_file, O_RDONLY); - ASSERT_NOERROR(relro_fd); - extinfo.flags = ANDROID_DLEXT_RESERVED_ADDRESS | ANDROID_DLEXT_USE_RELRO; - extinfo.relro_fd = relro_fd; - handle_ = android_dlopen_ext(LIBNAME, RTLD_NOW, &extinfo); - ASSERT_NOERROR(close(relro_fd)); + void CreateRelroFile(const char* lib) { + int relro_fd = open(relro_file_, O_CREAT | O_RDWR | O_TRUNC, 0644); + ASSERT_NOERROR(relro_fd); - ASSERT_DL_NOTNULL(handle_); - fn f = reinterpret_cast(dlsym(handle_, "getRandomNumber")); - ASSERT_DL_NOTNULL(f); - EXPECT_EQ(4, f()); + pid_t pid = fork(); + if (pid == 0) { + // child process + extinfo_.flags |= ANDROID_DLEXT_WRITE_RELRO; + extinfo_.relro_fd = relro_fd; + void* handle = android_dlopen_ext(lib, RTLD_NOW, &extinfo_); + if (handle == NULL) { + fprintf(stderr, "in child: %s\n", dlerror()); + exit(1); + } + exit(0); + } + + // continuing in parent + ASSERT_NOERROR(close(relro_fd)); + ASSERT_NOERROR(pid); + int status; + ASSERT_EQ(pid, waitpid(pid, &status, 0)); + ASSERT_TRUE(WIFEXITED(status)); + ASSERT_EQ(0, WEXITSTATUS(status)); + + // reopen file for reading so it can be used + relro_fd = open(relro_file_, O_RDONLY); + ASSERT_NOERROR(relro_fd); + extinfo_.flags |= ANDROID_DLEXT_USE_RELRO; + extinfo_.relro_fd = relro_fd; + } + + void TryUsingRelro(const char* lib) { + handle_ = android_dlopen_ext(lib, RTLD_NOW, &extinfo_); + ASSERT_DL_NOTNULL(handle_); + fn f = reinterpret_cast(dlsym(handle_, "getRandomNumber")); + ASSERT_DL_NOTNULL(f); + EXPECT_EQ(4, f()); + } + + android_dlextinfo extinfo_; + char relro_file_[PATH_MAX]; +}; + +TEST_F(DlExtRelroSharingTest, ChildWritesGoodData) { + ASSERT_NO_FATAL_FAILURE(CreateRelroFile(LIBNAME)); + ASSERT_NO_FATAL_FAILURE(TryUsingRelro(LIBNAME)); +} + +TEST_F(DlExtRelroSharingTest, ChildWritesNoRelro) { + ASSERT_NO_FATAL_FAILURE(CreateRelroFile(LIBNAME_NORELRO)); + ASSERT_NO_FATAL_FAILURE(TryUsingRelro(LIBNAME_NORELRO)); +} + +TEST_F(DlExtRelroSharingTest, RelroFileEmpty) { + int relro_fd = open(relro_file_, O_CREAT | O_RDWR | O_TRUNC, 0644); + ASSERT_NOERROR(relro_fd); + ASSERT_NOERROR(close(relro_fd)); + + ASSERT_NO_FATAL_FAILURE(TryUsingRelro(LIBNAME)); }