[ELF] .note.gnu.property: error for invalid pr_datasize

[MaskRay]

Summary

A n_type==NT_GNU_PROPERTY_TYPE_0 note encodes a program property.
If pr_datasize is invalid, LLD may crash
(https://github.com/ClangBuiltLinux/linux/issues/1141)

This patch adds the error checking, supports big-endian, and some tests
for invalid n_descsz.

[MaskRay]

n_descsz is too larget -> data is too short

[MaskRay]

Report the offset of .note.gnu.property

[psmith]

Overall looking good. I think there may be an endianness problem with reinterpret_cast although I could easily be wrong without testing (I don't think we have a target that uses .note.gnu.property with big-endian support in LLD).

lld/ELF/InputFiles.cpp

795–796

Will this work if the Host is little endian and the data in big-endian format?

797

We may be able to be more precise with the size as I think data.size() = sizeof(Elf_Nhdr) + nhdr->getSize()

[grimar]

lld/ELF/InputFiles.cpp

795–796

I believe yes. ELF_Nhdr is Elf_Nhdr_Impl:

template <class ELFT>
struct Elf_Nhdr_Impl {
  LLVM_ELF_IMPORT_TYPES_ELFT(ELFT)
  Elf_Word n_namesz;
  Elf_Word n_descsz;
  Elf_Word n_type;
...

And Elf_Word s here are using Word = packed<uint32_t> which respects
endianess:

struct packed_endian_specific_integral {
...
operator value_type() const {
  return endian::read<value_type, endian, alignment>(Ptr);
}

template <typename value_type, std::size_t alignment>
inline value_type read(const void *memory, endianness endian) {
  value_type ret;

  memcpy(&ret,
         LLVM_ASSUME_ALIGNED(
             memory, (detail::PickAlignment<value_type, alignment>::value)),
         sizeof(value_type));
  return byte_swap<value_type>(ret, endian);
}

template <typename value_type>
inline value_type byte_swap(value_type value, endianness endian) {
  if ((endian != native) && (endian != system_endianness()))
    sys::swapByteOrder(value);
  return value;
}

820

2 points:

1. You should be able to use read32 from Target.h:

inline uint32_t read32(const void *p) {
  return llvm::support::endian::read32(p, config->endianness);
}

2. It probably would read slightly better if it was:

if (desc.size() < 8)
  reportFatal(...);

uint32_t type = read32(desc.data());
uint32_t size = read32(desc.data() + 4);
desc = desc.drop_front(8);

if (desc.size() < size)
  reportFatal(desc, "program property is too short");

i.e
a) I'd not start reading the size + type when the data is truncated (just like the code on the left).
b) Early dropping of 8 bytes at start should allow to avoid the need of + 8 in the code below.

[grimar]

lld/ELF/InputFiles.cpp

820

I'd not start reading the size + type when the data is truncated

The code indeed does not starts reading them, I meant I'd just return earlier when it is already known that we can't read the header.

[MaskRay]

lld/ELF/InputFiles.cpp

797

This is precise: nhdr->getSize() = sizeof(Elf_Nhdr) + n_descsz.

820

1. This file does not include Target.h. I don't want to additional introduce an inter-module dependency. I think read32 with an explicit template argument is equally readable (slightly more efficient in performance and better codegen).

2. Changed.

[MaskRay]

Address comments

[grimar]

LGTM. Lets see what Peter think.

[psmith]

LGTM too, I'm happy my comments were addressed, thanks.

lld/ELF/InputFiles.cpp

 // Essentially we want to read a single 32-bit value in this function, but this
 // function is rather complicated because the value is buried deep inside a
 // .note.gnu.property section.
 //
 // The section consists of one or more NOTE records. Each NOTE record consists
 // of zero or more type-length-value fields. We want to find a field of a
 // certain type. It seems a bit too much to just store a 32-bit value, perhaps
 // the ABI is unnecessarily complicated.
-template <class ELFT>
-static uint32_t readAndFeatures(ObjFile<ELFT> *obj, ArrayRef<uint8_t> data) {
+template <class ELFT> static uint32_t readAndFeatures(const InputSection &sec) {
   using Elf_Nhdr = typename ELFT::Nhdr;
   using Elf_Note = typename ELFT::Note;
 
   uint32_t featuresSet = 0;
+  ArrayRef<uint8_t> data = sec.data();
+  auto reportFatal = [&](const uint8_t *place, const char *msg) {
+    fatal(toString(sec.file) + ":(" + sec.name + "+0x" +
+          Twine::utohexstr(place - sec.data().data()) + "): " + msg);
+  };
   while (!data.empty()) {
     // Read one NOTE record.
-    if (data.size() < sizeof(Elf_Nhdr))
-      fatal(toString(obj) + ": .note.gnu.property: section too short");
-
     auto *nhdr = reinterpret_cast<const Elf_Nhdr *>(data.data());
-    if (data.size() < nhdr->getSize())
-      fatal(toString(obj) + ": .note.gnu.property: section too short");
+    if (data.size() < sizeof(Elf_Nhdr) || data.size() < nhdr->getSize())
+      reportFatal(data.data(), "data is too short");
 
     Elf_Note note(*nhdr);
     if (nhdr->n_type != NT_GNU_PROPERTY_TYPE_0 || note.getName() != "GNU") {
       data = data.slice(nhdr->getSize());
       continue;
     }
 
     uint32_t featureAndType = config->emachine == EM_AARCH64
                                   ? GNU_PROPERTY_AARCH64_FEATURE_1_AND
                                   : GNU_PROPERTY_X86_FEATURE_1_AND;
 
     // Read a body of a NOTE record, which consists of type-length-value fields.
     ArrayRef<uint8_t> desc = note.getDesc();
     while (!desc.empty()) {
+      const uint8_t *place = desc.data();
       if (desc.size() < 8)
-        fatal(toString(obj) + ": .note.gnu.property: section too short");
-
-      uint32_t type = read32le(desc.data());
-      uint32_t size = read32le(desc.data() + 4);
+        reportFatal(place, "program property is too short");
+      uint32_t type = read32<ELFT::TargetEndianness>(desc.data());
+      uint32_t size = read32<ELFT::TargetEndianness>(desc.data() + 4);
+      desc = desc.slice(8);
+      if (desc.size() < size)
+        reportFatal(place, "program property is too short");
 
       if (type == featureAndType) {
         // We found a FEATURE_1_AND field. There may be more than one of these
         // in a .note.gnu.property section, for a relocatable object we
         // accumulate the bits set.
-        featuresSet |= read32le(desc.data() + 8);
+        if (size < 4)
+          reportFatal(place, "FEATURE_1_AND entry is too short");
+        featuresSet |= read32<ELFT::TargetEndianness>(desc.data());
       }
 
-      // On 64-bit, a payload may be followed by a 4-byte padding to make its
-      // size a multiple of 8.
-      if (ELFT::Is64Bits)
-        size = alignTo(size, 8);
-
-      desc = desc.slice(size + 8); // +8 for Type and Size
+      // Padding is present in the note descriptor, if necessary.
+      desc = desc.slice(alignTo<(ELFT::Is64Bits ? 8 : 4)>(size));
     }
 
     // Go to next NOTE record to look for more FEATURE_1_AND descriptions.
     data = data.slice(nhdr->getSize());
   }
 
   return featuresSet;
 }
InputSectionBase *ObjFile<ELFT>::createInputSection(const Elf_Shdr &sec) {
   // Enforcement (CET) or AArch64 Branch Target Identification BTI, use a
   // .note.gnu.property section containing a bitfield of feature bits like the
   // GNU_PROPERTY_X86_FEATURE_1_IBT flag. Read a bitmap containing the flag.
   //
   // Since we merge bitmaps from multiple object files to create a new
   // .note.gnu.property containing a single AND'ed bitmap, we discard an input
   // file's .note.gnu.property section.
   if (name == ".note.gnu.property") {
-    ArrayRef<uint8_t> contents = check(this->getObj().getSectionContents(&sec));
-    this->andFeatures = readAndFeatures(this, contents);
+    this->andFeatures = readAndFeatures<ELFT>(InputSection(*this, sec, name));
     return &InputSection::discarded;
   }
 
   // Split stacks is a feature to support a discontiguous stack,
   // commonly used in the programming language Go. For the details,
   // see https://gcc.gnu.org/wiki/SplitStacks. An object file compiled
   // for split stack will include a .note.GNU-split-stack section.
   if (name == ".note.GNU-split-stack") {

lld/test/ELF/gnu-property-err.s

+# REQUIRES: aarch64
+# RUN: split-file %s %t
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64 %t/1.s -o %t1.o
+# RUN: not ld.lld %t1.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR1
+
+# ERR1: error: {{.*}}.o:(.note.gnu.property+0x0): data is too short
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64 %t/2.s -o %t2.o
+# RUN: not ld.lld %t2.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR2
+# RUN: llvm-mc -filetype=obj -triple=aarch64_be %t/2.s -o %t2be.o
+# RUN: not ld.lld %t2be.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR2
+
+# ERR2: error: {{.*}}.o:(.note.gnu.property+0x10): program property is too short
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64 %t/3.s -o %t3.o
+# RUN: not ld.lld %t3.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR3
+# RUN: llvm-mc -filetype=obj -triple=aarch64_be %t/3.s -o %t3be.o
+# RUN: not ld.lld %t3be.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=ERR3
+
+# ERR3: error: {{.*}}.o:(.note.gnu.property+0x10): FEATURE_1_AND entry is too short
+
+#--- 1.s
+.section ".note.gnu.property", "a"
+.long 4
+.long 17         // n_descsz too long
+.long 5          // NT_GNU_PROPERTY_TYPE_0
+.asciz "GNU"
+
+.long 0xc0000000 // GNU_PROPERTY_AARCH64_FEATURE_1_AND
+.long 4          // pr_datasz
+.long 1          // GNU_PROPERTY_AARCH64_FEATURE_1_BTI
+.long 0
+
+#--- 2.s
+.section ".note.gnu.property", "a"
+.long 4
+.long 16         // n_descsz
+.long 5          // NT_GNU_PROPERTY_TYPE_0
+.asciz "GNU"
+
+.long 0xc0000000 // GNU_PROPERTY_AARCH64_FEATURE_1_AND
+.long 9          // pr_datasz too long
+.long 1          // GNU_PROPERTY_AARCH64_FEATURE_1_BTI
+.long 0
+
+#--- 3.s
+.section ".note.gnu.property", "a"
+.long 4
+.long 8          // n_descsz
+.long 5          // NT_GNU_PROPERTY_TYPE_0
+.asciz "GNU"
+
+.long 0xc0000000 // GNU_PROPERTY_AARCH64_FEATURE_1_AND
+.long 0          // pr_datasz too short