[llvm-readobj] - Don't crash when relocation table goes past the EOF.

[grimar]

Summary

It is possible to trigger reading past the EOF by breaking fields like
DT_PLTRELSZ, DT_RELSZ or DT_RELASZ

This patch adds a validation in DynRegionInfo helper class.

[jhenderson]

Would adding --implicit-check-not=warning: to the test cases be useful?

llvm/tools/llvm-readobj/ELFDumper.cpp

128–131

Can't you change users of this overload in the same way, so that there's always an object?

165

I think this punctuation is slightly more correct English.

[grimar]

• Addressed review comments.

[grimar]

In D91787#2407439, @jhenderson wrote:

Would adding --implicit-check-not=warning: to the test cases be useful?

Perhaps we could can add it to each test where we print warnings jsut in case.
I've added it.

llvm/tools/llvm-readobj/ELFDumper.cpp

128–131

I was not sure how much it is useful (because this constructor is only used in createDRI, which do Offset/Size check before creating the DynRegionInfo). But I've tried and seems it allows to simplify the code a bit.

Done.

[jhenderson]

LGTM, with or without the suggested changes.

llvm/tools/llvm-readobj/ELFDumper.cpp

141

Maybe make this a reference? It can't change after all, and is always valid.

160

Here and below, maybe reportUniqueWarning?

[grimar]

llvm/tools/llvm-readobj/ELFDumper.cpp

141

Would be good, but I can't, because the code stops to compile.
Compiler removes copy assignment operator when a class has a reference member:

function was implicitly deleted because '`anonymous-namespace'::DynRegionInfo' has a data member '`anonymous-namespace'::DynRegionInfo::Owner' of reference type

It is needed in one place where DynRegionInfo os used as Optional<>.

llvm/test/tools/llvm-readobj/ELF/broken-dynamic-reloc.test

 DynamicSymbols:
   - StName: 0x1234
 ProgramHeaders:
   - Type:     PT_LOAD
     FirstSec: .rela.dyn
     LastSec:  .dynamic
 
 ## Show we print a warning for an invalid relocation table size stored in a DT_RELASZ entry.
-# RUN: yaml2obj --docnum=2 -DRELTYPE=RELA -DTAG1=DT_RELASZ -DTAG1VAL=0xFF -DTAG2=DT_RELAENT %s -o %t2
-# RUN: llvm-readobj --dyn-relocations %t2 2>&1 | FileCheck %s -DFILE=%t2 --check-prefix=INVALID-DT-RELASZ
-# RUN: llvm-readelf --dyn-relocations %t2 2>&1 | FileCheck %s -DFILE=%t2 --check-prefix=INVALID-DT-RELASZ
 
-# INVALID-DT-RELASZ: warning: '[[FILE]]': invalid DT_RELASZ value (0xff) or DT_RELAENT value (0x18)
+## Case A: the size of a single relocation entry is 0x18. In this case 0xFF % 0x18 != 0 and we report a warning
+
+# RUN: yaml2obj --docnum=2 -DRELTYPE=RELA -DTAG1=DT_RELASZ -DTAG1VAL=0xFF -DTAG2=DT_RELAENT %s -o %t2a
+# RUN: llvm-readobj --dyn-relocations %t2a 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t2a --check-prefix=INVALID-DT-RELASZ1 --implicit-check-not=warning:
+# RUN: llvm-readelf --dyn-relocations %t2a 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t2a --check-prefix=INVALID-DT-RELASZ1 --implicit-check-not=warning:
+
+# INVALID-DT-RELASZ1: warning: '[[FILE]]': invalid DT_RELASZ value (0xff) or DT_RELAENT value (0x18)
 
 --- !ELF
 FileHeader:
   Class: ELFCLASS64
   Data:  ELFDATA2LSB
   Type:  ET_DYN
 Sections:
   - Name:  .relx.dyn
Entries:
       - Tag:   DT_NULL
         Value: 0x0
 DynamicSymbols: []
 ProgramHeaders:
   - Type:     PT_LOAD
     FirstSec: .relx.dyn
     LastSec:  .dynamic
 
+## Case B: the DT_RELASZ has value of 0x251, what is too large, because the relocation table goes past the EOF.
+
+# RUN: yaml2obj --docnum=2 -DRELTYPE=RELA -DTAG1=DT_RELASZ -DTAG1VAL=0x251 -DTAG2=DT_RELAENT %s -o %t2b
+# RUN: llvm-readobj --dyn-relocations %t2b 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t2b --check-prefix=INVALID-DT-RELASZ2 --implicit-check-not=warning:
+# RUN: llvm-readelf --dyn-relocations %t2b 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t2b --check-prefix=INVALID-DT-RELASZ2 --implicit-check-not=warning:
+
+# INVALID-DT-RELASZ2: warning: '[[FILE]]': unable to read data at 0x78 of size 0x251 (DT_RELASZ value): it goes past the end of the file of size 0x2c8
+
 ## Show we print a warning for an invalid relocation table entry size stored in a DT_RELAENT entry.
 # RUN: yaml2obj --docnum=2 -DRELTYPE=RELA -DTAG1=DT_RELASZ -DTAG2=DT_RELAENT -DTAG2VAL=0xFF %s -o %t3
-# RUN: llvm-readobj --dyn-relocations %t3 2>&1 | FileCheck %s -DFILE=%t3 --check-prefix=INVALID-DT-RELAENT
-# RUN: llvm-readelf --dyn-relocations %t3 2>&1 | FileCheck %s -DFILE=%t3 --check-prefix=INVALID-DT-RELAENT
+# RUN: llvm-readobj --dyn-relocations %t3 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t3 --check-prefix=INVALID-DT-RELAENT --implicit-check-not=warning:
+# RUN: llvm-readelf --dyn-relocations %t3 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t3 --check-prefix=INVALID-DT-RELAENT --implicit-check-not=warning:
 
 ## INVALID-DT-RELAENT: warning: '[[FILE]]': invalid DT_RELASZ value (0x18) or DT_RELAENT value (0xff)
 
 ## Show we print a warning for an invalid relocation table size stored in a DT_RELSZ entry.
-# RUN: yaml2obj --docnum=2 -DRELTYPE=REL -DTAG1=DT_RELSZ -DTAG1VAL=0xFF -DTAG2=DT_RELENT %s -o %t4
-# RUN: llvm-readobj --dyn-relocations %t4 2>&1 | FileCheck %s -DFILE=%t4 --check-prefix=INVALID-DT-RELSZ
-# RUN: llvm-readelf --dyn-relocations %t4 2>&1 | FileCheck %s -DFILE=%t4 --check-prefix=INVALID-DT-RELSZ
 
-## INVALID-DT-RELSZ: warning: '[[FILE]]': invalid DT_RELSZ value (0xff) or DT_RELENT value (0x18)
+## Case A: the size of a single relocation entry is 0x18. In this case 0xFF % 0x18 != 0 and we report a warning.
+
+# RUN: yaml2obj --docnum=2 -DRELTYPE=REL -DTAG1=DT_RELSZ -DTAG1VAL=0xFF -DTAG2=DT_RELENT %s -o %t4a
+# RUN: llvm-readobj --dyn-relocations %t4a 2>&1 | FileCheck %s -DFILE=%t4a --check-prefix=INVALID-DT-RELSZ1
+# RUN: llvm-readelf --dyn-relocations %t4a 2>&1 | FileCheck %s -DFILE=%t4a --check-prefix=INVALID-DT-RELSZ1
+
+## INVALID-DT-RELSZ1: warning: '[[FILE]]': invalid DT_RELSZ value (0xff) or DT_RELENT value (0x18)
+
+## Case B: the DT_RELSZ has value of 0x251, what is too large, because the relocation table goes past the EOF.
+
+# RUN: yaml2obj --docnum=2 -DRELTYPE=REL -DTAG1=DT_RELSZ -DTAG1VAL=0x251 -DTAG2=DT_RELENT %s -o %t4b
+# RUN: llvm-readobj --dyn-relocations %t4b 2>&1 | FileCheck %s -DFILE=%t4b --check-prefix=INVALID-DT-RELSZ2
+# RUN: llvm-readelf --dyn-relocations %t4b 2>&1 | FileCheck %s -DFILE=%t4b --check-prefix=INVALID-DT-RELSZ2
+
+# INVALID-DT-RELSZ2: warning: '[[FILE]]': unable to read data at 0x78 of size 0x251 (DT_RELSZ value): it goes past the end of the file of size 0x2c8
 
 ## Show we print a warning for an invalid relocation table entry size stored in a DT_RELENT entry.
 # RUN: yaml2obj --docnum=2 -DRELTYPE=REL -DTAG1=DT_RELSZ -DTAG2=DT_RELENT -DTAG2VAL=0xFF %s -o %t5
 # RUN: llvm-readobj --dyn-relocations %t5 2>&1 | FileCheck %s -DFILE=%t5 --check-prefix=INVALID-DT-RELENT
 # RUN: llvm-readelf --dyn-relocations %t5 2>&1 | FileCheck %s -DFILE=%t5 --check-prefix=INVALID-DT-RELENT
 
 ## INVALID-DT-RELENT: warning: '[[FILE]]': invalid DT_RELSZ value (0x18) or DT_RELENT value (0xff)
 

 # RUN: llvm-readobj --dyn-relocations %t9 2>&1 | FileCheck %s -DFILE=%t9 --check-prefix=INVALID-DT-ANDROID-RELRENT
 # RUN: llvm-readelf --dyn-relocations %t9 2>&1 | FileCheck %s -DFILE=%t9 --check-prefix=INVALID-DT-ANDROID-RELRENT
 
 ## INVALID-DT-RELRENT:         invalid DT_RELRSZ value (0x18) or DT_RELRENT value (0xff)
 ## INVALID-DT-ANDROID-RELRENT: invalid DT_ANDROID_RELRSZ value (0x18) or DT_ANDROID_RELRENT value (0xff)
 
 ## Show we print a warning for an invalid value of DT_PLTRELSZ, which describes the total size
 ## of the relocation entries associated with the procedure linkage table.
-# RUN: yaml2obj --docnum=3 %s -o %t10
-# RUN: llvm-readobj --dyn-relocations %t10 2>&1 | FileCheck %s -DFILE=%t10 --check-prefix=INVALID-DT-PLTRELSZ
-# RUN: llvm-readelf --dyn-relocations %t10 2>&1 | FileCheck %s -DFILE=%t10 --check-prefix=INVALID-DT-PLTRELSZ
 
-# INVALID-DT-PLTRELSZ: warning: '[[FILE]]': invalid DT_PLTRELSZ value (0xff){{$}}
+## Case A: the size of a single relocation entry is 0x18. In this case 0xFF % 0x18 != 0 and we report a warning.
+
+# RUN: yaml2obj --docnum=3 -DVAL=0xFF %s -o %t10a
+# RUN: llvm-readobj --dyn-relocations %t10a 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10a --check-prefix=INVALID-DT-PLTRELSZ1 --implicit-check-not=warning:
+# RUN: llvm-readelf --dyn-relocations %t10a 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10a --check-prefix=INVALID-DT-PLTRELSZ1 --implicit-check-not=warning:
+
+# INVALID-DT-PLTRELSZ1: warning: '[[FILE]]': invalid DT_PLTRELSZ value (0xff){{$}}
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name:  .rela.plt
     Type:  SHT_RELA
     Relocations:
       - Type:   R_X86_64_NONE
   - Name:  .dynamic
     Type:  SHT_DYNAMIC
     Entries:
       - Tag:   DT_JMPREL
         Value: 0x0
       - Tag:   DT_PLTRELSZ
-        Value: 0xFF ## The valid value would be 0x18.
+        Value: [[VAL]] ## The valid value would be 0x18.
       - Tag:   DT_PLTREL
         Value: 0x7 ## DT_RELA
       - Tag:   DT_NULL
         Value: 0x0
 DynamicSymbols: []
 ProgramHeaders:
   - Type:     PT_LOAD
     FirstSec: .rela.plt
     LastSec:  .dynamic
 
+## Case B: the DT_PLTRELSZ (PLT size) has value of 0x269, what is too large, because PLT goes past the EOF.
+
+# RUN: yaml2obj --docnum=3 -DVAL=0x269 %s -o %t10b
+# RUN: llvm-readobj --dyn-relocations %t10b 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10b --check-prefix=INVALID-DT-PLTRELSZ2-LLVM --implicit-check-not=warning:
+# RUN: llvm-readelf --dyn-relocations %t10b 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t10b --check-prefix=INVALID-DT-PLTRELSZ2-GNU --implicit-check-not=warning:
+
+# INVALID-DT-PLTRELSZ2-LLVM:      Dynamic Relocations {
+# INVALID-DT-PLTRELSZ2-LLVM-NEXT: warning: '[[FILE]]': unable to read data at 0x78 of size 0x269 (DT_PLTRELSZ value): it goes past the end of the file of size 0x2e0
+# INVALID-DT-PLTRELSZ2-LLVM-NEXT: }
+
+# INVALID-DT-PLTRELSZ2-GNU:      'PLT' relocation section at offset 0x78 contains 617 bytes:
+# INVALID-DT-PLTRELSZ2-GNU-NEXT:     Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
+# INVALID-DT-PLTRELSZ2-GNU-NEXT: warning: '[[FILE]]': unable to read data at 0x78 of size 0x269 (DT_PLTRELSZ value): it goes past the end of the file of size 0x2e0
+
 ## Show we print a warning when dumping dynamic relocations if there is no dynamic symbol table.
 # RUN: yaml2obj --docnum=4 %s -o %t11
 # RUN: llvm-readobj --dyn-relocations %t11 2>&1 | FileCheck %s -DFILE=%t11 --check-prefix=LLVM-NO-DYNSYM
 # RUN: llvm-readelf --dyn-relocations %t11 2>&1 | FileCheck %s -DFILE=%t11 --check-prefix=GNU-NO-DYNSYM
 
 # LLVM-NO-DYNSYM:      Dynamic Relocations {
 # LLVM-NO-DYNSYM-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 0: no dynamic symbol table found
 # LLVM-NO-DYNSYM-NEXT:   0x0 R_X86_64_NONE <corrupt> 0x0

llvm/tools/llvm-readobj/ELFDumper.cpp

template <class ELFT> struct RelSymbol {
   const typename ELFT::Sym *Sym;
   std::string Name;
 };
 
 /// Represents a contiguous uniform range in the file. We cannot just create a
 /// range directly because when creating one of these from the .dynamic table
 /// the size, entity size and virtual address are different entries in arbitrary
 /// order (DT_REL, DT_RELSZ, DT_RELENT for example).
 struct DynRegionInfo {
-  DynRegionInfo(StringRef ObjName) : FileName(ObjName) {}
-  DynRegionInfo(const uint8_t *A, uint64_t S, uint64_t ES, StringRef ObjName)
-      : Addr(A), Size(S), EntSize(ES), FileName(ObjName) {}
+  DynRegionInfo(const Binary &Owner) : Obj(&Owner) {}
+  DynRegionInfo(const Binary &Owner, const uint8_t *A, uint64_t S, uint64_t ES)
+      : Addr(A), Size(S), EntSize(ES), Obj(&Owner) {}
 
   /// Address in current address space.
   const uint8_t *Addr = nullptr;
   /// Size in bytes of the region.
   uint64_t Size = 0;
   /// Size of each entity in the region.
   uint64_t EntSize = 0;
 
-  /// Name of the file. Used for error reporting.
-  StringRef FileName;
+  /// Owner object. Used for error reporting.
+  const Binary *Obj;
   /// Error prefix. Used for error reporting to provide more information.
   std::string Context;
   /// Region size name. Used for error reporting.
   StringRef SizePrintName = "size";
   /// Entry size name. Used for error reporting. If this field is empty, errors
   /// will not mention the entry size.
   StringRef EntSizePrintName = "entry size";
 
   template <typename Type> ArrayRef<Type> getAsArrayRef() const {
     const Type *Start = reinterpret_cast<const Type *>(Addr);
     if (!Start)
       return {Start, Start};
+
+    const uint64_t Offset =
+        Addr - (const uint8_t *)Obj->getMemoryBufferRef().getBufferStart();
+    const uint64_t ObjSize = Obj->getMemoryBufferRef().getBufferSize();
+
+    if (Size > ObjSize - Offset) {
+      reportWarning(
+          createError("unable to read data at 0x" + Twine::utohexstr(Offset) +
+                      " of size 0x" + Twine::utohexstr(Size) + " (" +
+                      SizePrintName +
+                      "): it goes past the end of the file of size 0x" +
+                      Twine::utohexstr(ObjSize)),
+          Obj->getFileName());
+      return {Start, Start};
+    }
+
     if (EntSize == sizeof(Type) && (Size % EntSize == 0))
       return {Start, Start + (Size / EntSize)};
 
     std::string Msg;
     if (!Context.empty())
       Msg += Context + " has ";
 
     Msg += ("invalid " + SizePrintName + " (0x" + Twine::utohexstr(Size) + ")")
                .str();
     if (!EntSizePrintName.empty())
       Msg +=
           (" or " + EntSizePrintName + " (0x" + Twine::utohexstr(EntSize) + ")")
               .str();
 
-    reportWarning(createError(Msg.c_str()), FileName);
+    reportWarning(createError(Msg.c_str()), Obj->getFileName());
     return {Start, Start};
   }
 };
 
 struct GroupMember {
   StringRef Name;
   uint64_t Index;
 };
private:
 
   Expected<DynRegionInfo> createDRI(uint64_t Offset, uint64_t Size,
                                     uint64_t EntSize) {
     if (Offset + Size < Offset || Offset + Size > Obj.getBufSize())
       return createError("offset (0x" + Twine::utohexstr(Offset) +
                          ") + size (0x" + Twine::utohexstr(Size) +
                          ") is greater than the file size (0x" +
                          Twine::utohexstr(Obj.getBufSize()) + ")");
-    return DynRegionInfo(Obj.base() + Offset, Size, EntSize,
-                         ObjF.getFileName());
+    return DynRegionInfo(ObjF, Obj.base() + Offset, Size, EntSize);
   }
 
   void printAttributes();
   void printMipsReginfo();
   void printMipsOptions();
 
   std::pair<const Elf_Phdr *, const Elf_Shdr *> findDynamic();
   void loadDynamicTable();

 template <typename ELFT>
 void ELFDumper<ELFT>::loadDynamicTable() {
   const Elf_Phdr *DynamicPhdr;
   const Elf_Shdr *DynamicSec;
   std::tie(DynamicPhdr, DynamicSec) = findDynamic();
   if (!DynamicPhdr && !DynamicSec)
     return;
 
-  DynRegionInfo FromPhdr(ObjF.getFileName());
+  DynRegionInfo FromPhdr(ObjF);
   bool IsPhdrTableValid = false;
   if (DynamicPhdr) {
     // Use cantFail(), because p_offset/p_filesz fields of a PT_DYNAMIC are
     // validated in findDynamic() and so createDRI() is not expected to fail.
     FromPhdr = cantFail(createDRI(DynamicPhdr->p_offset, DynamicPhdr->p_filesz,
                                   sizeof(Elf_Dyn)));
     FromPhdr.SizePrintName = "PT_DYNAMIC size";
     FromPhdr.EntSizePrintName = "";
     IsPhdrTableValid = !FromPhdr.getAsArrayRef<Elf_Dyn>().empty();
   }
 
   // Locate the dynamic table described in a section header.
   // Ignore sh_entsize and use the expected value for entry size explicitly.
   // This allows us to dump dynamic sections with a broken sh_entsize
   // field.
-  DynRegionInfo FromSec(ObjF.getFileName());
+  DynRegionInfo FromSec(ObjF);
   bool IsSecTableValid = false;
   if (DynamicSec) {
     Expected<DynRegionInfo> RegOrErr =
         createDRI(DynamicSec->sh_offset, DynamicSec->sh_size, sizeof(Elf_Dyn));
     if (RegOrErr) {
       FromSec = *RegOrErr;
       FromSec.Context = describe(*DynamicSec);
       FromSec.EntSizePrintName = "";
void ELFDumper<ELFT>::loadDynamicTable() {
   }
 
   parseDynamicTable();
 }
 
 template <typename ELFT>
 ELFDumper<ELFT>::ELFDumper(const object::ELFObjectFile<ELFT> &O,
                            ScopedPrinter &Writer)
-    : ObjDumper(Writer), ObjF(O), Obj(*O.getELFFile()),
-      DynRelRegion(O.getFileName()), DynRelaRegion(O.getFileName()),
-      DynRelrRegion(O.getFileName()), DynPLTRelRegion(O.getFileName()),
-      DynamicTable(O.getFileName()) {
+    : ObjDumper(Writer), ObjF(O), Obj(*O.getELFFile()), DynRelRegion(O),
+      DynRelaRegion(O), DynRelrRegion(O), DynPLTRelRegion(O), DynamicTable(O) {
   // Dumper reports all non-critical errors as warnings.
   // It does not print the same warning more than once.
   WarningHandler = [this](const Twine &Msg) {
     if (Warnings.insert(Msg.str()).second)
       reportWarning(createError(Msg), ObjF.getFileName());
     return Error::success();
   };
 
case ELF::DT_STRTAB:
       break;
     case ELF::DT_STRSZ:
       StringTableSize = Dyn.getVal();
       break;
     case ELF::DT_SYMTAB: {
       // If we can't map the DT_SYMTAB value to an address (e.g. when there are
       // no program headers), we ignore its value.
       if (const uint8_t *VA = toMappedAddr(Dyn.getTag(), Dyn.getPtr())) {
-        DynSymFromTable.emplace(ObjF.getFileName());
+        DynSymFromTable.emplace(ObjF);
         DynSymFromTable->Addr = VA;
         DynSymFromTable->EntSize = sizeof(Elf_Sym);
         DynSymFromTable->EntSizePrintName = "";
       }
       break;
     }
     case ELF::DT_SYMENT: {
       uint64_t Val = Dyn.getVal();