simpleperf: fix potential unaligned memory access.

1. It is reported that when elf section headers are malformed,
GetBuildIdFromNoteSection() aborts with SIGBUS. So fix it to
accept not 4 bytes aligned section data.
2. Fix potential unaligned memory access in ConvertBytesToValue().

Test: run simpleperf_unit_test.
Change-Id: I2e1612a6567978e0b526b2274377765ba2837ec2

diff --git a/simpleperf/read_elf.cpp b/simpleperf/read_elf.cpp
index a969253..8f2f0b2 100644 (file)
--- a/simpleperf/read_elf.cpp
+++ b/simpleperf/read_elf.cpp
@@ -99,23 +99,30 @@ ElfStatus IsValidElfPath(const std::string& filename) {
   return result;
 }
 
-static bool GetBuildIdFromNoteSection(const char* section, size_t section_size, BuildId* build_id) {
+bool GetBuildIdFromNoteSection(const char* section, size_t section_size, BuildId* build_id) {
   const char* p = section;
   const char* end = p + section_size;
   while (p < end) {
-    CHECK_LE(p + 12, end);
-    size_t namesz = *reinterpret_cast<const uint32_t*>(p);
-    p += 4;
-    size_t descsz = *reinterpret_cast<const uint32_t*>(p);
-    p += 4;
-    uint32_t type = *reinterpret_cast<const uint32_t*>(p);
-    p += 4;
+    if (p + 12 >= end) {
+      return false;
+    }
+    uint32_t namesz;
+    uint32_t descsz;
+    uint32_t type;
+    MoveFromBinaryFormat(namesz, p);
+    MoveFromBinaryFormat(descsz, p);
+    MoveFromBinaryFormat(type, p);
     namesz = Align(namesz, 4);
     descsz = Align(descsz, 4);
-    CHECK_LE(p + namesz + descsz, end);
-    if ((type == NT_GNU_BUILD_ID) && (strcmp(p, ELF_NOTE_GNU) == 0)) {
-      *build_id = BuildId(p + namesz, descsz);
-      return true;
+    if ((type == NT_GNU_BUILD_ID) && (p < end) && (strcmp(p, ELF_NOTE_GNU) == 0)) {
+      const char* desc_start = p + namesz;
+      const char* desc_end = desc_start + descsz;
+      if (desc_start > p && desc_start < desc_end && desc_end <= end) {
+        *build_id = BuildId(p + namesz, descsz);
+        return true;
+      } else {
+        return false;
+      }
     }
     p += namesz + descsz;
   }

diff --git a/simpleperf/read_elf.h b/simpleperf/read_elf.h
index 5a916ad..2ea0131 100644 (file)
--- a/simpleperf/read_elf.h
+++ b/simpleperf/read_elf.h
@@ -77,5 +77,6 @@ ElfStatus ReadSectionFromElfFile(const std::string& filename, const std::string&
 bool IsArmMappingSymbol(const char* name);
 ElfStatus IsValidElfFile(int fd);
 ElfStatus IsValidElfPath(const std::string& filename);
+bool GetBuildIdFromNoteSection(const char* section, size_t section_size, BuildId* build_id);
 
 #endif  // SIMPLE_PERF_READ_ELF_H_

diff --git a/simpleperf/read_elf_test.cpp b/simpleperf/read_elf_test.cpp
index f60b552..c72a146 100644 (file)
--- a/simpleperf/read_elf_test.cpp
+++ b/simpleperf/read_elf_test.cpp
@@ -25,6 +25,40 @@
 
 #include "get_test_data.h"
 #include "test_util.h"
+#include "utils.h"
+
+#define ELF_NOTE_GNU "GNU"
+#define NT_GNU_BUILD_ID 3
+
+TEST(read_elf, GetBuildIdFromNoteSection) {
+  BuildId build_id;
+  std::vector<char> data;
+  // Fail to read build id for no data.
+  ASSERT_FALSE(GetBuildIdFromNoteSection(data.data(), 0, &build_id));
+
+  // Read build id from data starting from different alignment addresses.
+  char build_id_data[20];
+  for (int i = 0; i < 20; ++i) {
+    build_id_data[i] = i;
+  }
+  BuildId expected_build_id(build_id_data, 20);
+  data.resize(100, '\0');
+
+  for (size_t alignment = 0; alignment <= 3; ++alignment) {
+    char* start = data.data() + alignment;
+    char* p = start;
+    uint32_t type = NT_GNU_BUILD_ID;
+    uint32_t namesz = 4;
+    uint32_t descsz = 20;
+    MoveToBinaryFormat(namesz, p);
+    MoveToBinaryFormat(descsz, p);
+    MoveToBinaryFormat(type, p);
+    MoveToBinaryFormat(ELF_NOTE_GNU, 4, p);
+    MoveToBinaryFormat(build_id_data, 20, p);
+    ASSERT_TRUE(GetBuildIdFromNoteSection(start, p - start, &build_id));
+    ASSERT_TRUE(build_id == expected_build_id);
+  }
+}
 
 TEST(read_elf, GetBuildIdFromElfFile) {
   BuildId build_id;

diff --git a/simpleperf/utils.cpp b/simpleperf/utils.cpp
index e2d25aa..62a8b63 100644 (file)
--- a/simpleperf/utils.cpp
+++ b/simpleperf/utils.cpp
@@ -318,18 +318,17 @@ size_t GetPageSize() {
 }
 
 uint64_t ConvertBytesToValue(const char* bytes, uint32_t size) {
-  switch (size) {
-    case 1:
-      return *reinterpret_cast<const uint8_t*>(bytes);
-    case 2:
-      return *reinterpret_cast<const uint16_t*>(bytes);
-    case 4:
-      return *reinterpret_cast<const uint32_t*>(bytes);
-    case 8:
-      return *reinterpret_cast<const uint64_t*>(bytes);
+  if (size > 8) {
+    LOG(FATAL) << "unexpected size " << size << " in ConvertBytesToValue";
   }
-  LOG(FATAL) << "unexpected size " << size << " in ConvertBytesToValue";
-  return 0;
+  uint64_t result = 0;
+  int shift = 0;
+  for (uint32_t i = 0; i < size; ++i) {
+    uint64_t tmp = static_cast<unsigned char>(bytes[i]);
+    result |= tmp << shift;
+    shift += 8;
+  }
+  return result;
 }
 
 timeval SecondToTimeval(double time_in_sec) {

diff --git a/simpleperf/utils_test.cpp b/simpleperf/utils_test.cpp
index 23c669e..e08917c 100644 (file)
--- a/simpleperf/utils_test.cpp
+++ b/simpleperf/utils_test.cpp
@@ -35,7 +35,7 @@ static bool KernelSymbolsMatch(const KernelSymbol& sym1,
          ModulesMatch(sym1.module, sym2.module);
 }
 
-TEST(environment, ProcessKernelSymbols) {
+TEST(utils, ProcessKernelSymbols) {
   std::string data =
       "ffffffffa005c4e4 d __warned.41698   [libsas]\n"
       "aaaaaaaaaaaaaaaa T _text\n"
@@ -62,3 +62,14 @@ TEST(environment, ProcessKernelSymbols) {
       data,
       std::bind(&KernelSymbolsMatch, std::placeholders::_1, expected_symbol)));
 }
+
+TEST(utils, ConvertBytesToValue) {
+  char buf[8];
+  for (int i = 0; i < 8; ++i) {
+    buf[i] = i;
+  }
+  ASSERT_EQ(0x1ULL, ConvertBytesToValue(buf + 1, 1));
+  ASSERT_EQ(0x201ULL, ConvertBytesToValue(buf + 1, 2));
+  ASSERT_EQ(0x05040302ULL, ConvertBytesToValue(buf + 2, 4));
+  ASSERT_EQ(0x0706050403020100ULL, ConvertBytesToValue(buf, 8));
+}