Use a relative address in .note.crashpad.info

The desc value in the note is now the offset of CRASHPAD_INFO_SYMBOL
from desc.

Making this note writable can trigger a linker error resulting in
the binary embedding .note.crashpad.info to be rejected by the
kernel during program loading.

The error was observed with:
GNU ld (GNU Binutils for Debian) 2.30
clang version 4.0.1-10 (tags/RELEASE_401/final)
Debian 4.17.17-1rodete2

When the note is made writable, crashpad_snapshot_test contains two
PT_LOAD segments which map to the same page.

LOAD         0x0000000000000000 0x0000000000000000 0x0000000000000000
             0x0000000000000258 0x0000000000000258  R      0x200000
LOAD         0x0000000000000258 0x0000000000000258 0x0000000000000258
             0x00000000002b84d8 0x00000000002b8950  RWE    0x200000

Executing this binary with the execv system call triggers a segfault
during program loading (an error can't be returned because the original
process vm has already been discarded).

I suspect (I haven't set up a debuggable kernel) the failure occurs
while attempting to map the second load segment because its virtual
address, 0x258, is in the same page as the first load segment.
https://elixir.bootlin.com/linux/v4.17.17/source/fs/binfmt_elf.c#L380

The linker normally produces consecutive load segments where the second
segment is loaded 0x200000 bytes after the first, which I think is the
maximum expected page size. Modifying the test executable to load the
second segment at 0x1258 (4096 byte page size) allows program loading
to succeed (but of course crashes after control is given to it).

Bug: crashpad:260
Change-Id: I2b9f1e66e98919138baef3da991a9710bd970dc4
Reviewed-on: https://chromium-review.googlesource.com/c/1292232
Reviewed-by: Scott Graham <scottmg@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Joshua Peraza <jperaza@chromium.org>

client/crashpad_info_note.S

@@ -26,9 +26,11 @@
 #define NOTE_ALIGN 4
 
   // This section must be "a"llocated so that it appears in the final binary at
-  // runtime, and "w"ritable so that the relocation to CRASHPAD_INFO_SYMBOL can
-  // be performed.
-  .section .note.crashpad.info,"aw",%note
+  // runtime. The reference to CRASHPAD_INFO_SYMBOL uses an offset relative to
+  // this note to avoid making this note writable, which triggers a bug in GNU
+  // ld, or adding text relocations which require the target system to allow
+  // making text segments writable. https://crbug.com/crashpad/260.
+  .section .note.crashpad.info,"a",%note
   .balign NOTE_ALIGN
   # .globl indicates that it's available to link against other .o files. .hidden
   # indicates that it will not appear in the executable's symbol table.
@@ -45,15 +47,9 @@ name_end:
   .balign NOTE_ALIGN
 desc:
 #if defined(__LP64__)
-  .quad CRASHPAD_INFO_SYMBOL
+  .quad CRASHPAD_INFO_SYMBOL - desc
 #else
-#if defined(__LITTLE_ENDIAN__)
-  .long CRASHPAD_INFO_SYMBOL
-  .long 0
-#else
-  .long 0
-  .long CRASHPAD_INFO_SYMBOL
-#endif  // __LITTLE_ENDIAN__
+  .long CRASHPAD_INFO_SYMBOL - desc
 #endif  // __LP64__
 desc_end:
   .size CRASHPAD_NOTE_REFERENCE, .-CRASHPAD_NOTE_REFERENCE

snapshot/crashpad_info_size_test_note.S

@@ -26,9 +26,11 @@
 #define NOTE_ALIGN 4
 
   // This section must be "a"llocated so that it appears in the final binary at
-  // runtime, and "w"ritable so that the relocation to TEST_CRASHPAD_INFO_SYMBOL
-  // can be performed.
-  .section .note.crashpad.info,"aw",%note
+  // runtime. The reference to TEST_CRASHPAD_INFO_SYMBOL uses an offset relative
+  // to this note to avoid making this note writable, which triggers a bug in
+  // GNU ld, or adding text relocations which require the target system to allow
+  // making text segments writable. https://crbug.com/crashpad/260.
+  .section .note.crashpad.info,"a",%note
   .balign NOTE_ALIGN
   .type info_size_test_note, %object
 info_size_test_note:
@@ -41,15 +43,9 @@ name_end:
   .balign NOTE_ALIGN
 desc:
 #if defined(__LP64__)
-  .quad TEST_CRASHPAD_INFO_SYMBOL
+  .quad TEST_CRASHPAD_INFO_SYMBOL - desc
 #else
-#if defined(__LITTLE_ENDIAN__)
-  .long TEST_CRASHPAD_INFO_SYMBOL
-  .long 0
-#else
-  .long 0
-  .long TEST_CRASHPAD_INFO_SYMBOL
-#endif  // __LITTLE_ENDIAN__
+  .long TEST_CRASHPAD_INFO_SYMBOL - desc
 #endif  // __LP64__
 desc_end:
   .size info_size_test_note, .-info_size_test_note

snapshot/elf/elf_image_reader.cc

@@ -191,7 +191,8 @@ ElfImageReader::NoteReader::~NoteReader() = default;
 ElfImageReader::NoteReader::Result ElfImageReader::NoteReader::NextNote(
     std::string* name,
     NoteType* type,
-    std::string* desc) {
+    std::string* desc,
+    VMAddress* desc_address) {
   if (!is_valid_) {
     LOG(ERROR) << "invalid note reader";
     return Result::kError;
@@ -215,8 +216,9 @@ ElfImageReader::NoteReader::Result ElfImageReader::NoteReader::NextNote(
     }
 
     retry_ = false;
-    result = range_->Is64Bit() ? ReadNote<Elf64_Nhdr>(name, type, desc)
-                               : ReadNote<Elf32_Nhdr>(name, type, desc);
+    result = range_->Is64Bit()
+                 ? ReadNote<Elf64_Nhdr>(name, type, desc, desc_address)
+                 : ReadNote<Elf32_Nhdr>(name, type, desc, desc_address);
   } while (retry_);
 
   if (result == Result::kSuccess) {
@@ -251,7 +253,8 @@ template <typename NhdrType>
 ElfImageReader::NoteReader::Result ElfImageReader::NoteReader::ReadNote(
     std::string* name,
     NoteType* type,
-    std::string* desc) {
+    std::string* desc,
+    VMAddress* desc_address) {
   static_assert(sizeof(*type) >= sizeof(NhdrType::n_namesz),
                 "Note field size mismatch");
   DCHECK_LT(current_address_, segment_end_address_);
@@ -317,6 +320,7 @@ ElfImageReader::NoteReader::Result ElfImageReader::NoteReader::ReadNote(
           current_address_, note_info.n_descsz, &local_desc[0])) {
     return Result::kError;
   }
+  *desc_address = current_address_;
 
   current_address_ = end_of_note;
 

snapshot/elf/elf_image_reader.h

@@ -70,9 +70,14 @@ class ElfImageReader {
     //! \param[out] name The name of the note owner, if not `nullptr`.
     //! \param[out] type A type for the note, if not `nullptr`.
     //! \param[out] desc The note descriptor.
-    //! \return a #Result value. \a name, \a type, and \a desc are only valid if
-    //!     this method returns Result::kSuccess.
-    Result NextNote(std::string* name, NoteType* type, std::string* desc);
+    //! \param[out] desc_addr The address in the remote process' address space
+    //!     \a desc was read from.
+    //! \return a #Result value. \a name, \a type, \a desc, and \a desc_addr are
+    //!     only valid if this method returns Result::kSuccess.
+    Result NextNote(std::string* name,
+                    NoteType* type,
+                    std::string* desc,
+                    VMAddress* desc_addr);
 
     // private
     NoteReader(const ElfImageReader* elf_reader_,
@@ -88,7 +93,10 @@ class ElfImageReader {
     // and returns kError if use_filter_ is true and the note's name and type do
     // not match name_filter_ and type_filter_.
     template <typename T>
-    Result ReadNote(std::string* name, NoteType* type, std::string* desc);
+    Result ReadNote(std::string* name,
+                    NoteType* type,
+                    std::string* desc,
+                    VMAddress* desc_addr);
 
     VMAddress current_address_;
     VMAddress segment_end_address_;

snapshot/elf/elf_image_reader_fuzzer.cc

@@ -65,8 +65,10 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   std::string note_name;
   std::string note_desc;
   ElfImageReader::NoteReader::NoteType note_type;
+  VMAddress desc_addr;
   auto notes = reader.Notes(-1);
-  while ((result = notes->NextNote(&note_name, &note_type, &note_desc)) ==
+  while ((result = notes->NextNote(
+              &note_name, &note_type, &note_desc, &desc_addr)) ==
          ElfImageReader::NoteReader::Result::kSuccess) {
     LOG(ERROR) << note_name << note_type << note_desc;
   }

snapshot/elf/elf_image_reader_test.cc

@@ -154,22 +154,24 @@ void ReadThisExecutableInTarget(ProcessType process,
   std::string note_name;
   std::string note_desc;
   ElfImageReader::NoteReader::NoteType note_type;
+  VMAddress desc_addr;
 
   std::unique_ptr<ElfImageReader::NoteReader> notes = reader.Notes(-1);
-  while ((result = notes->NextNote(&note_name, &note_type, &note_desc)) ==
+  while ((result = notes->NextNote(
+              &note_name, &note_type, &note_desc, &desc_addr)) ==
          ElfImageReader::NoteReader::Result::kSuccess) {
   }
   EXPECT_EQ(result, ElfImageReader::NoteReader::Result::kNoMoreNotes);
 
   notes = reader.Notes(0);
-  EXPECT_EQ(notes->NextNote(&note_name, &note_type, &note_desc),
+  EXPECT_EQ(notes->NextNote(&note_name, &note_type, &note_desc, &desc_addr),
             ElfImageReader::NoteReader::Result::kNoMoreNotes);
 
   // Find the note defined in elf_image_reader_test_note.S.
   constexpr uint32_t kCrashpadNoteDesc = 42;
   notes = reader.NotesWithNameAndType(
       CRASHPAD_ELF_NOTE_NAME, CRASHPAD_ELF_NOTE_TYPE_SNAPSHOT_TEST, -1);
-  ASSERT_EQ(notes->NextNote(&note_name, &note_type, &note_desc),
+  ASSERT_EQ(notes->NextNote(&note_name, &note_type, &note_desc, &desc_addr),
             ElfImageReader::NoteReader::Result::kSuccess);
   EXPECT_EQ(note_name, CRASHPAD_ELF_NOTE_NAME);
   EXPECT_EQ(note_type,
@@ -178,7 +180,7 @@ void ReadThisExecutableInTarget(ProcessType process,
   EXPECT_EQ(*reinterpret_cast<decltype(kCrashpadNoteDesc)*>(&note_desc[0]),
             kCrashpadNoteDesc);
 
-  EXPECT_EQ(notes->NextNote(&note_name, &note_type, &note_desc),
+  EXPECT_EQ(notes->NextNote(&note_name, &note_type, &note_desc, &desc_addr),
             ElfImageReader::NoteReader::Result::kNoMoreNotes);
 }
 

snapshot/elf/module_snapshot_elf.cc

@@ -56,9 +56,17 @@ bool ModuleSnapshotElf::Initialize() {
                                         kMaxNoteSize);
   std::string desc;
   VMAddress info_address;
-  if (notes->NextNote(nullptr, nullptr, &desc) ==
+  VMAddress desc_address;
+  if (notes->NextNote(nullptr, nullptr, &desc, &desc_address) ==
       ElfImageReader::NoteReader::Result::kSuccess) {
-    info_address = *reinterpret_cast<VMAddress*>(&desc[0]);
+    VMOffset offset;
+    if (elf_reader_->Memory()->Is64Bit()) {
+      offset = *reinterpret_cast<VMOffset*>(&desc[0]);
+    } else {
+      int32_t offset32 = *reinterpret_cast<int32_t*>(&desc[0]);
+      offset = offset32;
+    }
+    info_address = desc_address + offset;
 
     ProcessMemoryRange range;
     if (range.Initialize(*elf_reader_->Memory())) {
@@ -145,7 +153,8 @@ void ModuleSnapshotElf::UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const {
   std::unique_ptr<ElfImageReader::NoteReader> notes =
       elf_reader_->NotesWithNameAndType(ELF_NOTE_GNU, NT_GNU_BUILD_ID, 64);
   std::string desc;
-  notes->NextNote(nullptr, nullptr, &desc);
+  VMAddress desc_addr;
+  notes->NextNote(nullptr, nullptr, &desc, &desc_addr);
   desc.insert(desc.end(), 16 - std::min(desc.size(), size_t{16}), '\0');
   uuid->InitializeFromBytes(reinterpret_cast<const uint8_t*>(&desc[0]));